Add more debugging around new project to debug failure#430
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughIncrease command execution timeout to 2 minutes; trim and conditionally log command stdout; log exit code and elapsed duration when available; detect deadline exceeded and log timeout with elapsed; on errors log concrete error type; for Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as TemplateStep
participant Runner as steps.go
participant OS as OS/Process
Caller->>Runner: Execute command (timeout = 2m)
Runner->>Runner: record startTime
Runner->>OS: start process
OS-->>Runner: stdout, exit status, process state
Runner->>Runner: buf = trim(stdout)
alt ProcessState present
Runner->>Runner: log exit code and elapsed
end
alt buf non-empty
Runner->>Caller: log "Command output: <buf>"
end
alt success
Runner->>Caller: return success
else timeout
Runner->>Caller: log timeout with elapsed, return error (uses buf)
else error
Runner->>Caller: log error value and concrete type
alt command == "uv add" and exit code == 130
Runner->>OS: run "uv pip install" (Stdin=nil, timeout)
OS-->>Runner: result
alt failed
Runner->>OS: run "pip install" (Stdin=nil, timeout)
OS-->>Runner: result
end
end
Runner->>Caller: return error (trimmed buf or aggregated error)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
internal/templates/steps.go (3)
46-46: Make timeout configurable; avoid hard-coded magic numberBumping to 2 minutes is reasonable for debugging, but encode this as a constant or a configurable value to avoid scattered magic numbers and to tune without code changes.
Apply within the selected line range:
-timeoutCtx, cancel := context.WithTimeout(ctx.Context, 2*time.Minute) +timeoutCtx, cancel := context.WithTimeout(ctx.Context, stepCmdTimeout)Add this near the top of the file (outside the selected range):
const stepCmdTimeout = 2 * time.Minute
54-60: Cap and sanitize logged command output to prevent excessive logs and accidental secret leakageLogging the full CombinedOutput can flood logs for verbose commands. Truncate to a safe upper bound.
Apply this diff inside the block:
buf := strings.TrimSpace(string(out)) if buf != "" { + const maxLogBytes = 32 * 1024 + if len(buf) > maxLogBytes { + buf = buf[:maxLogBytes] + fmt.Sprintf("... [truncated %d bytes]", len(buf)-maxLogBytes) + } ctx.Logger.Debug("Command output: %s", buf) if cmd.ProcessState != nil { ctx.Logger.Debug("command exit code %d", cmd.ProcessState.ExitCode()) } }
98-98: Include exit code in returned error for quicker diagnosisYou already log exit code in debug; include it in the returned error to surface the signal/status in upstream logs and UIs.
- return fmt.Errorf("failed to run command: %s with args: %s: %w (%s)", s.Command, strings.Join(s.Args, " "), err, buf) + exitCode := -1 + if cmd.ProcessState != nil { + exitCode = cmd.ProcessState.ExitCode() + } + return fmt.Errorf("failed to run command: %s with args: %s: %w (exit=%d, out=%s)", + s.Command, strings.Join(s.Args, " "), err, exitCode, buf)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/templates/steps.go(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and Test (macos-latest)
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
internal/templates/steps.go (1)
63-63: Nice: log error type alongside valueIncluding the concrete error type greatly aids debugging. Good addition.
Summary by CodeRabbit
Bug Fixes
Chores